Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Accept null dlhandle and support custom dlopen flags #35

Closed
wants to merge 12 commits into from
Closed

Accept null dlhandle and support custom dlopen flags #35

wants to merge 12 commits into from

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Feb 5, 2021 •
edited
Loading

After digging through #30, I was able to get ign-sensors to work with only this change, without touching the local / global variables. It seems to me that this change is backwards compatible, because it only tries the nullptr after trying the usual method first. Update: this PR also allows changing setting to global.

We know no previous use cases were getting a null handle because we haven't run into that assert. So I think this is safe to go into ign-plugin1.

Unfortunately, I have no idea what leads us into this situation, so I'm not able to write a test case for it 😕

Let me know what you think, @ahcorde .

Here's the ign-sensors PR that uses this: gazebosim/gz-sensors#90

@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Feb 5, 2021
@codecov
Copy link

codecov bot commented Feb 5, 2021 •
edited
Loading

Codecov Report

Merging #35 (0317f4c) into ign-plugin1 (a80598b) will decrease coverage by 1.18%.
The diff coverage is 30.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-plugin1      #35      +/-   ##
===============================================
- Coverage        99.82%   98.64%   -1.19%     
===============================================
  Files               15       15              
  Lines              584      592       +8     
===============================================
+ Hits               583      584       +1     
- Misses               1        8       +7
Impacted Files Coverage Δ
loader/src/Loader.cc 96.75% <30.00%> (-3.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a80598b...54e2981. Read the comment docs.

This was referenced Feb 5, 2021
Merged
Closed
Closed
Closed
@ahcorde
Copy link
Contributor

ahcorde commented Feb 5, 2021

I made a cimment about this here gazebosim/gz-sensors#90 (comment)

@chapulina chapulina mentioned this pull request Feb 6, 2021
Merged
8 tasks
@chapulina
Customizable flags
fd77c57 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina changed the title Accept null dlhandle Accept null dlhandle and support custom dlopen flags Feb 10, 2021
@chapulina
Fix doc
9368f71 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

I made a cimment about this here

@ahcorde , I think the current version of this PR addresses those test failures. This PR adds support for RTLD_GLOBAL just like #30 does, with the advantage of:

  • not requiring a version bump
  • allowing the flexibility of not applying the change to all libraries
  • allowing the flexibility of restoring the RTLD_LOCAL behaviour in the future if we want

Let me know what you think.

@chapulina
Add tests - segfaulting
54e2981 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

I started writing some tests using RLTD_GLOBAL in 54e2981 and it's not looking good. Tests crash on shutdown, crash when loading libraries depending on the flag combination.

I'm afraid that we get this in, start using RTLD_GLOBAL, and start seeing runtime failures throughout the stack.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021
@chapulina chapulina self-assigned this Mar 19, 2021
@chapulina chapulina removed the beta Targeting beta release of upcoming collection label Mar 23, 2021
@chapulina
Copy link
Contributor Author

Removing beta label, we won't have time to wrap this up before code freeze. Let's retarget at Ignition-F.

@chapulina
Copy link
Contributor Author

Let's table this idea for now, tweaking the flags is probably not the best way to go about this.

@chapulina chapulina closed this May 13, 2021
@chapulina chapulina mentioned this pull request Aug 16, 2022
Merged
9 tasks
@scpeters scpeters deleted the chapulina/global/symbols branch March 6, 2023 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkoenig nkoenig nkoenig approved these changes

@ahcorde ahcorde ahcorde approved these changes

@mxgrey mxgrey Awaiting requested review from mxgrey

Assignees

@chapulina chapulina

Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chapulina @ahcorde @nkoenig